balancer/randomsubsetting: Implementation of the random_subsetting LB policy #8650

marek-szews · 2025-10-14T11:44:18Z

Implements [gRFC A68]https://github.com/grpc/proposal/blob/master/A68-random-subsetting.md.

Note that this PR only implements the LB policy and does not implement the xDS integration specified here: https://github.com/grpc/proposal/blob/master/A68-random-subsetting.md#xds-integration

RELEASE NOTES:

balancer/randomsubsetting: Implementation of the random_subsetting LB policy

Signed-off-by: marek-szews <[email protected]>

linux-foundation-easycla · 2025-10-14T11:44:24Z

The committers listed above are authorized under a signed CLA.

✅ login: easwars / name: Easwar Swaminathan (74cfecd, c4f8448, ddc8d09, f577218)
✅ login: marek-szews / name: Marek Szews (3c3f093, 7f43d9e, e8ff4a1)

easwars · 2025-10-15T19:39:09Z

@marek-szews : Could you please get the tests to pass. Thanks.

balancer/randomsubsetting/randomsubsetting.go

easwars · 2025-10-15T19:52:14Z

balancer/randomsubsetting/randomsubsetting.go

+	}
+
+	// if someonw needs subsetSize == 1, he should use pick_first instead
+	if lbCfg.SubsetSize < 2 {


The gRFC only says the following:

// The value is required and must be greater than 0.

Is the condition that the size be at least 2 specified somewhere else?

See the discussion at line 86

Line 86 of what?

The only thing we should be checking is whether the subset size is greater than 0 and if not, returning an error from this method.

balancer/randomsubsetting/randomsubsetting.go

github-actions · 2025-10-23T08:49:39Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

dfawley · 2025-10-29T16:04:09Z

Hi @marek-szews - I think we're waiting on you to make some updates so we can continue the review. We can discuss Friday but I wanted to ping this in part to prevent the stale bot from closing it.

marek-szews

Rework done, tests passed

balancer/randomsubsetting/randomsubsetting.go

easwars · 2025-11-13T20:58:57Z

Rework done, tests passed

Tests are still failing. Please read this document for guidelines: https://github.com/grpc/grpc-go/blob/master/CONTRIBUTING.md

TL;dr

All tests need to be passing before your change can be merged. We recommend you run tests locally before creating your PR to catch breakages early on:

./scripts/vet.sh to catch vet errors.
go test -cpu 1,4 -timeout 7m ./... to run the tests.
go test -race -cpu 1,4 -timeout 7m ./... to run tests in race mode

… fields

… a prefix logger

easwars · 2025-11-13T21:37:21Z

balancer/randomsubsetting/randomsubsetting.go

+	}
+
+	if b.cfg == nil || b.cfg.ChildPolicy.Name != lbCfg.ChildPolicy.Name {
+


Please remove this newline.

easwars · 2025-11-13T21:38:53Z

balancer/randomsubsetting/randomsubsetting.go

+type lbConfig struct {
+	serviceconfig.LoadBalancingConfig `json:"-"`
+
+	SubsetSize  uint64                         `json:"subsetSize,omitempty"`


This needs to be uint32 as per the gRFC.

easwars · 2025-11-13T21:39:41Z

balancer/randomsubsetting/randomsubsetting.go

+func (b *subsettingBalancer) prepareChildResolverState(s balancer.ClientConnState) resolver.State {
+	subsetSize := b.cfg.SubsetSize
+	endPoints := s.ResolverState.Endpoints
+	backendCount := len(endPoints)


Nit: Please get rid of the backendCount local variable. The cide is more readable when used as len(endpoints).

easwars · 2025-11-13T21:40:03Z

balancer/randomsubsetting/randomsubsetting.go

+// as described in A68: https://github.com/grpc/proposal/blob/master/A68-random-subsetting.md
+func (b *subsettingBalancer) prepareChildResolverState(s balancer.ClientConnState) resolver.State {
+	subsetSize := b.cfg.SubsetSize
+	endPoints := s.ResolverState.Endpoints


Nit: s/endPoints/endpoints/

endpoint is a single word.

easwars · 2025-11-13T21:42:19Z

balancer/randomsubsetting/randomsubsetting.go

+
+	// sort endpoint by hash
+	slices.SortFunc(endpointSet, func(a, b endpointWithHash) int {
+		return cmp.Compare(a.hash, b.hash)


Please do not use the cmp package in non-test code. The cmp package is only meant for test code. Please use the < operator here as the hash field is a uint64

easwars · 2025-11-13T21:52:34Z

balancer/randomsubsetting/randomsubsetting_test.go

+					// The backend receives a second request from the same client. This could happen if the client have 1 backend in READY
+					// state while the other  are CONNECTING. In this case round_robbin will pick the same address twice.
+					// We are going to retry after short timeout.
+					time.Sleep(10 * time.Microsecond)


Please avoid sleeping in tests.

easwars · 2025-11-13T21:53:35Z

balancer/randomsubsetting/randomsubsetting_test.go

+	return clients, cancel
+}
+
+func checkRoundRobinRPCs(ctx context.Context, t *testing.T, clients []testgrpc.TestServiceClient, subsetSize int, maxDiff int) {


We have something like this already:

grpc-go/internal/testutils/roundrobin/roundrobin.go

Line 85 in e816736

func CheckRoundRobinRPCs(ctx context.Context, client testgrpc.TestServiceClient, addrs []resolver.Address) error {

Would you be able to use that instead? Thanks

easwars · 2025-11-13T21:57:49Z

balancer/randomsubsetting/randomsubsetting_test.go

+	cancel := func() {
+		for _, cc := range ccs {
+			cc.Close()
+		}
+	}


You could use t.Cleanup here as well and avoid having to return a cancel function that the test needs to invoke.

easwars · 2025-11-13T22:00:02Z

balancer/randomsubsetting/randomsubsetting_test.go

+		maxDiff    int
+	}{
+		{
+			name:       "backends could be evenly distributed between small number of clients",


Please see here for guidelines on how best to name subtests: https://google.github.io/styleguide/go/decisions#subtest-names

TL;dr
Think of subtest names more like a function identifier than a prose description.

The test runner replaces spaces with underscores, and escapes non-printing characters. To ensure accurate correlation between test logs and source code, it is recommended to avoid using these characters in subtest names.

easwars · 2025-11-13T22:03:58Z

balancer/randomsubsetting/randomsubsetting.go

+func (bb) Build(cc balancer.ClientConn, bOpts balancer.BuildOptions) balancer.Balancer {
+	b := &subsettingBalancer{
+		Balancer: gracefulswitch.NewBalancer(cc, bOpts),
+		hashf:    xxhash.NewWithSeed(uint64(time.Now().UnixNano())),


You probably want to use a random number here as specified in the gRFC. Also, it would be useful to be able to override the random value in tests for predictability. Here is an example of how to do what I'm suggesting:

grpc-go/internal/wrr/random.go

Line 49 in c0de489

var randInt64n = rand.Int64N

Implementation of A68 random_subsetting LB policy.

7f43d9e

Signed-off-by: marek-szews <[email protected]>

Change a coments

e8ff4a1

easwars self-assigned this Oct 15, 2025

easwars self-requested a review October 15, 2025 19:17

easwars assigned marek-szews and unassigned easwars Oct 15, 2025

easwars added the Status: Requires Reporter Clarification label Oct 15, 2025

easwars reviewed Oct 15, 2025

View reviewed changes

github-actions bot added stale and removed stale labels Oct 23, 2025

arjan-bal added this to the 1.78 Release milestone Oct 30, 2025

rework for A68

3c3f093

marek-szews commented Oct 31, 2025

View reviewed changes

dfawley assigned easwars and unassigned marek-szews Oct 31, 2025

dfawley removed the Status: Requires Reporter Clarification label Oct 31, 2025

easwars changed the title ~~Implementation of A68 random_subsetting LB policy.~~ balancer/randomsubsetting: Implementation of the LB policy Nov 13, 2025

easwars changed the title ~~balancer/randomsubsetting: Implementation of the LB policy~~ balancer/randomsubsetting: Implementation of the random_subsetting LB policy Nov 13, 2025

easwars assigned marek-szews and unassigned easwars Nov 13, 2025

embed the gracefulswitch.Balancer in subsettingBalancer

c4f8448

easwars added 3 commits November 13, 2025 21:07

use camelCase instead of snake_case in json annotations for LB config…

f577218

… fields

do not use the package name prefix in log messages, as we already use…

ddc8d09

… a prefix logger

improvements to ParseConfig and its tests

74cfecd

easwars reviewed Nov 13, 2025

View reviewed changes

		}

		if b.cfg == nil \|\| b.cfg.ChildPolicy.Name != lbCfg.ChildPolicy.Name {

balancer/randomsubsetting: Implementation of the random_subsetting LB policy #8650

Are you sure you want to change the base?

balancer/randomsubsetting: Implementation of the random_subsetting LB policy #8650

Uh oh!

Conversation

marek-szews commented Oct 14, 2025 • edited by easwars Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

easwars commented Oct 15, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 23, 2025

Uh oh!

dfawley commented Oct 29, 2025

Uh oh!

marek-szews left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

easwars commented Nov 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marek-szews commented Oct 14, 2025 •

edited by easwars

Loading

linux-foundation-easycla bot commented Oct 14, 2025 •

edited

Loading